CDAP-21264 : Respect custom CPU and memory multipliers for Task Workers - #16196
CDAP-21264 : Respect custom CPU and memory multipliers for Task Workers#16196sahusanket wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes Kubernetes CPU and memory multiplier constants in Constants.java and updates TaskWorkerServiceLauncher, KubeTwillPreparer, and PreviewServiceMain to utilize them. Feedback suggests optimizing TaskWorkerServiceLauncher by storing configuration lookups in local variables to avoid redundant calls, replacing duplicate local constants in KubeTwillPreparer with the new centralized constants, and correcting a minor typo in the Javadoc of Constants.java.
| if (cConf.get(Constants.TaskWorker.CONTAINER_CPU_MULTIPLIER) != null) { | ||
| configMap.put(Constants.Kube.CPU_MULTIPLIER, | ||
| cConf.get(Constants.TaskWorker.CONTAINER_CPU_MULTIPLIER)); | ||
| } | ||
| if (cConf.get(Constants.TaskWorker.CONTAINER_MEMORY_MULTIPLIER) != null) { | ||
| configMap.put(Constants.Kube.MEMORY_MULTIPLIER, | ||
| cConf.get(Constants.TaskWorker.CONTAINER_MEMORY_MULTIPLIER)); | ||
| } |
There was a problem hiding this comment.
To avoid redundant lookups in cConf (which can be expensive as it resolves configuration variables), store the retrieved multiplier values in local variables instead of calling cConf.get() multiple times for the same key.
| if (cConf.get(Constants.TaskWorker.CONTAINER_CPU_MULTIPLIER) != null) { | |
| configMap.put(Constants.Kube.CPU_MULTIPLIER, | |
| cConf.get(Constants.TaskWorker.CONTAINER_CPU_MULTIPLIER)); | |
| } | |
| if (cConf.get(Constants.TaskWorker.CONTAINER_MEMORY_MULTIPLIER) != null) { | |
| configMap.put(Constants.Kube.MEMORY_MULTIPLIER, | |
| cConf.get(Constants.TaskWorker.CONTAINER_MEMORY_MULTIPLIER)); | |
| } | |
| String cpuMultiplier = cConf.get(Constants.TaskWorker.CONTAINER_CPU_MULTIPLIER); | |
| if (cpuMultiplier != null) { | |
| configMap.put(Constants.Kube.CPU_MULTIPLIER, cpuMultiplier); | |
| } | |
| String memoryMultiplier = cConf.get(Constants.TaskWorker.CONTAINER_MEMORY_MULTIPLIER); | |
| if (memoryMultiplier != null) { | |
| configMap.put(Constants.Kube.MEMORY_MULTIPLIER, memoryMultiplier); | |
| } |
| if (config.containsKey(CPU_MULTIPLIER)) { | ||
| systemCpuMultiplier = config.get(CPU_MULTIPLIER); | ||
| } | ||
| if (config.containsKey(MEMORY_MULTIPLIER)) { | ||
| systemMemoryMultiplier = config.get(MEMORY_MULTIPLIER); | ||
| } |
There was a problem hiding this comment.
Instead of using the locally defined CPU_MULTIPLIER and MEMORY_MULTIPLIER constants (which duplicates the configuration keys), use the newly introduced centralized constants Constants.Kube.CPU_MULTIPLIER and Constants.Kube.MEMORY_MULTIPLIER from cdap-common to ensure consistency and maintainability.
| if (config.containsKey(CPU_MULTIPLIER)) { | |
| systemCpuMultiplier = config.get(CPU_MULTIPLIER); | |
| } | |
| if (config.containsKey(MEMORY_MULTIPLIER)) { | |
| systemMemoryMultiplier = config.get(MEMORY_MULTIPLIER); | |
| } | |
| if (config.containsKey(Constants.Kube.CPU_MULTIPLIER)) { | |
| systemCpuMultiplier = config.get(Constants.Kube.CPU_MULTIPLIER); | |
| } | |
| if (config.containsKey(Constants.Kube.MEMORY_MULTIPLIER)) { | |
| systemMemoryMultiplier = config.get(Constants.Kube.MEMORY_MULTIPLIER); | |
| } |
| float cpuMultiplier = Float.parseFloat(systemCpuMultiplier != null ? systemCpuMultiplier : | ||
| cConf.getOrDefault(CPU_MULTIPLIER, DEFAULT_MULTIPLIER)); | ||
| float memoryMultiplier = Float.parseFloat(systemMemoryMultiplier != null ? systemMemoryMultiplier : | ||
| cConf.getOrDefault(MEMORY_MULTIPLIER, DEFAULT_MULTIPLIER)); |
There was a problem hiding this comment.
Use the centralized constants Constants.Kube.CPU_MULTIPLIER and Constants.Kube.MEMORY_MULTIPLIER here as well to avoid relying on duplicate local constants.
| float cpuMultiplier = Float.parseFloat(systemCpuMultiplier != null ? systemCpuMultiplier : | |
| cConf.getOrDefault(CPU_MULTIPLIER, DEFAULT_MULTIPLIER)); | |
| float memoryMultiplier = Float.parseFloat(systemMemoryMultiplier != null ? systemMemoryMultiplier : | |
| cConf.getOrDefault(MEMORY_MULTIPLIER, DEFAULT_MULTIPLIER)); | |
| float cpuMultiplier = Float.parseFloat(systemCpuMultiplier != null ? systemCpuMultiplier : | |
| cConf.getOrDefault(Constants.Kube.CPU_MULTIPLIER, DEFAULT_MULTIPLIER)); | |
| float memoryMultiplier = Float.parseFloat(systemMemoryMultiplier != null ? systemMemoryMultiplier : | |
| cConf.getOrDefault(Constants.Kube.MEMORY_MULTIPLIER, DEFAULT_MULTIPLIER)); |
| /** | ||
| * Kubernetes constants. | ||
| * These same as defined in KubeTwillPreparer | ||
| */ |
There was a problem hiding this comment.
Fix the grammatical typo in the Javadoc comment and update it to reflect that these are the centralized constants used by KubeTwillPreparer.
| /** | |
| * Kubernetes constants. | |
| * These same as defined in KubeTwillPreparer | |
| */ | |
| /** | |
| * Kubernetes constants. | |
| * These are used by KubeTwillPreparer. | |
| */ |
|
1 participant
No description provided.